Do not make the service name in the header a link if no serviceUrl is provided
#2617
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
36degrees
commented
May 13, 2022
Comment on lines
+145
to
+147
| // The govuk-header__link--service-name class is deprecated - use | ||
| // govuk-header__service-name instead. | ||
| .govuk-header__service-name, |
There was a problem hiding this comment.
Are we happy with this approach? If so, will need to create an issue to remove the govuk-header__link--service-name class and attach it to the milestone for v5.0, and include this in the release notes as a deprecation.
The alternative I think is to either:
- live with having a link modifier class on something that isn't a link (which feels icky)
- keep both classes around forever
There was a problem hiding this comment.
I've created an issue to remove the deprecated class here: #2636
36degrees
force-pushed
the
header-service-name-no-url
branch
from
071cb98
to
926b7e6
Compare
owenatgov
approved these changes
May 20, 2022
|
Afterthought: Does this need a changelog entry? |
|
Yep! I also need to create an issue to tidy up that deprecated class. |
36degrees
changed the title
href is provided
The header macro currently assumes that if there is a serviceName then there will always be a serviceUrl. If no serviceUrl is provided then the service name is wrapped with a link with an empty `href` attribute. There’s not always a logical place to link the service name in the header to, so it makes sense to allow for service names that don’t link anywhere. Update the macro so that when serviceUrl is omitted from the options a span is used to wrap the service name rather than a link. As the current class used on the service name is a modifier for links (`govuk-header__link--service-name`) but will not be used on a link in this context, rename it to `govuk-header__service-name` to make it more generic. Keep the old class name around as an alias to prevent this being a breaking change, but mark it as deprecated.
36degrees
force-pushed
the
header-service-name-no-url
branch
from
926b7e6
to
d265856
Compare
36degrees
changed the title
href is providedserviceUrl is provided
2 tasks
36degrees
force-pushed
the
header-service-name-no-url
branch
from
d265856
to
2248df7
Compare
Co-authored-by: EoinShaughnessy <[email protected]>
36degrees
force-pushed
the
header-service-name-no-url
branch
from
d14d981
to
2fb7589
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The header macro currently assumes that if there is a serviceName then there will always be a serviceUrl. If no serviceUrl is provided then the service name is wrapped with a link with an empty
hrefattribute.There’s not always a logical place to link the service name in the header to, so it makes sense to allow for service names that don’t link anywhere.
Update the macro so that when serviceUrl is omitted from the options a span is used to wrap the service name rather than a link.
As the current class used on the service name is a modifier for links (
govuk-header__link--service-name) but will not be used on a link in this context, rename it togovuk-header__service-nameto make it more generic.Keep the old class name around as an alias to prevent this being a breaking change, but mark it as deprecated.
Fixes #1826